Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use isbindingresolved when checking imported packages #21580

Merged
merged 1 commit into from
May 20, 2017
Merged

Use isbindingresolved when checking imported packages #21580

merged 1 commit into from
May 20, 2017
+11 −1

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Apr 27, 2017 •
edited
Loading

Fixes #21578

Marking as WIP until I can figure out how best to test this. Suggestions welcome.

Edit: Tested. 😎

@ararslan ararslan added needs tests Unit tests are required for this change packages Package management and loading labels Apr 27, 2017
@ararslan ararslan requested review from vchuravy and tkelman April 27, 2017 02:37
@ararslan
Copy link
Member Author

ararslan commented May 5, 2017

If ever I can figure out how to test this, should the change be backported to 0.6?

@helgee
Copy link
Contributor

helgee commented May 16, 2017 •
edited
Loading

Just for my understanding: What is the rationale for this change? And related, what is wrong with the original test?

Edit: Reading helps 😝

@vchuravy vchuravy self-assigned this May 16, 2017
vchuravy
vchuravy approved these changes May 19, 2017
View reviewed changes
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a notoriously difficult to test edge case since you need a binding in Base that is latent. The change per se is correct. But I haven't thought up a way of properly testing it besides doing Pkg.add("Iterators")....

@tkelman
Copy link
Contributor

tkelman commented May 19, 2017

we can do Pkg.add("Iterators") during the pkg test and test for other things, if that's the easiest way

@ararslan
Copy link
Member Author

The problem is that just adding Iterators doesn't trigger it—Iterators has to be updated for it to happen.

@tkelman
Copy link
Contributor

tkelman commented May 19, 2017

there is a part of the pkg test that does an update between 2 known points in metadata history, was there an Iterators update between them?

@ararslan ararslan force-pushed the aa/iterators-warn branch from 3d503df to d9522e4 Compare May 19, 2017 20:54
@ararslan
Copy link
Member Author

Got it tested! 🎉

@tkelman
Copy link
Contributor

tkelman commented May 19, 2017

and that would fail without the change to base?

@ararslan
Copy link
Member Author

Yep, tried it with and without.

@ararslan ararslan changed the title WIP: Use isbindingresolved when checking imported packages Use isbindingresolved when checking imported packages May 20, 2017
@ararslan ararslan removed the needs tests Unit tests are required for this change label May 20, 2017
@tkelman tkelman merged commit d762038 into master May 20, 2017
@tkelman tkelman deleted the aa/iterators-warn branch May 20, 2017 03:19
@tkelman tkelman mentioned this pull request May 21, 2017
Closed
tkelman pushed a commit that referenced this pull request Jun 3, 2017
@ararslan @tkelman
Use isbindingresolved when checking imported packages (#21580)
2b4d7fe 
(cherry picked from commit d762038)
@vtjnash
Copy link
Member

vtjnash commented Jun 13, 2017

This appears to have broken the coverage buildbot (http://buildbot.e.ip.saba.us:8010/builders/coverage_ubuntu14.04-x64/builds/357/steps/Run%20inlined%20tests/logs/stdio)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy approved these changes

@tkelman tkelman Awaiting requested review from tkelman

Assignees

@vchuravy vchuravy

Labels
packages Package management and loading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ararslan @helgee @tkelman @vtjnash @vchuravy